Skip to content

[algorithm] Add some checks for null BaseProximity::SPtr#65

Merged
epernod merged 8 commits into
masterfrom
add-checks
Aug 18, 2025
Merged

[algorithm] Add some checks for null BaseProximity::SPtr#65
epernod merged 8 commits into
masterfrom
add-checks

Conversation

@th-skam

@th-skam th-skam commented Aug 11, 2025

Copy link
Copy Markdown
Collaborator

Sometimes the operations that return proximity pointers could be null. Some checks are put in place before they are used.

if (dot(tip2Pt, normal) > 0_sreal) {
m_couplingPts.pop_back();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if edgeProx is null? should we sent a warning? or exit the loop?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed 2 warnings eventually.

  1. Notifies when shaftProx is null. It's unlikely because the CreateCenterProximity operations are straightforward.
  2. Notifies when edgeProx is null. With a valid shaftProx, this will happen if l_shaftGeom is not an EdgeGeometry.

If the two repos were merged into one, we could use the normal handler from the ConstraintGeometry repo. It could help avoid this downcasting.

@epernod epernod added pr: enhancement pr: status to review To notify reviewers to review this pull-request labels Aug 13, 2025
@epernod epernod added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Aug 18, 2025
@epernod epernod merged commit a047216 into master Aug 18, 2025
4 checks passed
@epernod epernod deleted the add-checks branch August 18, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants